Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bring back iOS application delegates and fix many issues #6453

Merged
merged 5 commits into from
Dec 15, 2024

Conversation

frenzibyte
Copy link
Member

After going through separate issues that ask for having access to a custom app delegate class, and specifically after #6449 / libsdl-org/SDL#11627, I've looked into bringing app delegate classes back.

Reading the SDL's codebase and various PRs in the repo, I've noticed that SDL intentionally doesn't do anything special/internal for its API in the app delegate class it uses for the application. This is to allow consumers that wish for a custom app delegate class to create one without extra effort (although I had to do this 3134295).

In order to use SDL without their own app delegate class, the SDL_RunApp should be omitted and we should call UIApplication.Main on our app delegate class directly instead, and at finishedLaunching, we just have to call SDL_SetMainReady to mark it as ready for initialisation.

For detailed reasons as to why this PR exists:

  • SDL disables the UIKit event pump after we return from the main function call that used to exist as GameApplication.main. Disabling the UIKit event pump causes the application to not receive will/did enter background/foreground events, therefore never suspending/unfocusing when sent to background.
    • We have also been doing this ourselves, but after quick discord inquiry, it was decided that it can be safely removed, and so it is 3ef7542.
    • While this can be fixed at SDL's side by just asking them to not disable the thing or do something about it, we can just have our own app delegate class and call it a day.
  • We need a UIApplicationDelegate class to control the list of allowed orientations, to add support for Disable screen rotation while in active gameplay osu#5331
  • I am of firm belief that a UIApplicationDelegate class is mandatory to have in any application for full flexibility over the app's configuration/behaviour, as well as handling various app-level events which may otherwise require asking SDL to add while not being of priority to them.

@smoogipoo
Copy link
Contributor

smoogipoo commented Dec 13, 2024

I think this is fine conceptually, because this is the default sort of implementation one would expect of Xcode's templates etc... Curious if @Susko3 has any thoughts (even though you're a bit more on the Android side of things).

Copy link
Member

@Susko3 Susko3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the hint SDL_HINT_IOS_HIDE_HOME_INDICATOR do anything now?

public override bool OpenUrl(UIApplication application, NSUrl url, string sourceApplication, NSObject annotation)
{
// copied verbatim from SDL: https://github.com/libsdl-org/SDL/blob/d252a8fe126b998bd1b0f4e4cf52312cd11de378/src/video/uikit/SDL_uikitappdelegate.m#L508-L535
// the hope is that the SDL app delegate class does not have such handling exist there, but Apple does not provide a corresponding notification to make that possible.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some copy-paste error in this comment? I don't really understand what it's supposed to say.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no error, there are just some iOS terminologies mixed in the comment. Apple provides things called Notifications in the iOS frameworks which allow applications to intercept events happening on any existing NSObject (i.e. any object), as long as said object exposes said Notifications (there's more into this but I don't want to dwell any further).

The key point of this comment is that some functionalities that used to exist in SDLUIKitDelegate were moved out to "observer" notification handlers to keep SDL away from the AppDelegate class. See libsdl-org/SDL#6011 (comment). However, there's no equivalent notification for handling OpenUrl outside of AppDelegate class, hence the existence of this comment and the copy-pasted code.


namespace TemplateGame.iOS
{
/// <inheritdoc />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is [Register("AppDelegate")] missing here?

Why is it required in the first place?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The attribute is inserted by default into every created iOS project, I did not delve deep into why it's required but I wouldn't want to defy what .NET/Xamarin does so /shrug. Will fix the missing specification.

Copy link
Member

@Susko3 Susko3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm mostly worried about SDL changing their SDLUIKitDelegate and us not following with that change. It's also possible that SDL stops working if it's not given a SDLUIKitDelegate.

In any case, if this goes in, someone should probably point their RSS reader at https://github.com/libsdl-org/SDL/commits/main/src/video/uikit/SDL_uikitappdelegate.m.atom and watch for any breaking changes.

return 0;
public override bool OpenUrl(UIApplication application, NSUrl url, string sourceApplication, NSObject annotation)
{
// copied verbatim from SDL: https://github.com/libsdl-org/SDL/blob/d252a8fe126b998bd1b0f4e4cf52312cd11de378/src/video/uikit/SDL_uikitappdelegate.m#L508-L535
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you subclass SDLUIKitDelegate? Seems to be supported by SDL. Maybe we don't want the extra baggage that comes from the SDL implementation?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the hope at first but it was an extremely complicated process. We haven't explored through the concept of "iOS binding projects" before, which is an essential key to expose such types in the C# world. And throughout a brief attempt from my end, it cannot be integrated within the SDL3-CS project, it has to be in a separate project, either SDL3-CS.iOSBindings or osu.Framework.iOS.SDLBindings, depending on what the end goal with the iOS binding project turns out.

It will be a multi-day effort with back-and-fourth discussions on where and how should it be implemented and whether it's going to cause issues if it's a separate project (looping back to the concern about the Android project being previously separate), so I ultimately gave up on it on the trust that SDL will not silently add crucial code in SDLUIKitDelegate.

@frenzibyte
Copy link
Member Author

I'm mostly worried about SDL changing their SDLUIKitDelegate and us not following with that change. It's also possible that SDL stops working if it's not given a SDLUIKitDelegate.

It looks to me that the entire SDL system is completely detached from the running app delegate class. I don't see the type mentioned anywhere other than in the file itself, and the current implementation of that class shows almost nothing crucial to the initialisation of the system (save for the URL handling code, which doesn't seem like it can be done any other way). I have set up the linked RSS feed to see if SDL will tell me otherwise.

@frenzibyte
Copy link
Member Author

Does the hint SDL_HINT_IOS_HIDE_HOME_INDICATOR do anything now?

That hint is handled by a separate component SDL_uikitviewcontroller which we are and will continue to be using: https://github.com/libsdl-org/SDL/blob/6cc9ce183d904489bf8e33e26b91d6012667e1b0/src/video/uikit/SDL_uikitviewcontroller.m#L46-L61

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants